refactor(major): Refactor entire india banking client side flows #13
Conversation
refactor: update install file path
refactor: single payment code add in bank connector
fix: add filter to the bank account fix: fetch company bank account details during the API call
fix: rename method action in payload
refactor: optimise payment request overrides code
…t entry and journal entry account
fix: add condition payment order type
india_banking/default.py
Outdated
| "maximum_limit": 200000, | ||
| "start_time": "0:00:00", | ||
| "end_time": "23:59:59", | ||
| "disabled": 1, |
There was a problem hiding this comment.
Why is every mode of transfer, by default disabled?
india_banking/default.py
Outdated
| { | ||
| "mode": "RTGS", | ||
| "minimum_limit": 200000, | ||
| "maximum_limit": 50000000, |
There was a problem hiding this comment.
Make 50 Crore as the upper limit, beyond that LEI number is required.
india_banking/default.py
Outdated
| { | ||
| "mode": "NEFT", | ||
| "minimum_limit": 0, | ||
| "maximum_limit": 100000000000, |
There was a problem hiding this comment.
Make 50 Crore as the upper limit, beyond that LEI number is required.
india_banking/default.py
Outdated
| "start_time": "0:00:00", | ||
| "end_time": "23:59:59", | ||
| "disabled": 1, | ||
| "priority": "1", |
There was a problem hiding this comment.
This is account specific. Please bring in that key here. And do the following priority.
- A2A
- IMPS
- RTGS
- NEFT
by default. Let the user re-order as per their need.
india_banking/default.py
Outdated
| ] | ||
|
|
||
| STD_BANK_LIST = [ | ||
| "Yes Bank", |
There was a problem hiding this comment.
Remove Yes Bank as of now. Reorder this in alphabetical order.
|
|
||
| after_install = ["india_banking.install.after_install"] | ||
|
|
||
| before_uninstall = "india_banking.uninstall.before_uninstall" |
There was a problem hiding this comment.
Can you put this in array too? Is it supported?
There was a problem hiding this comment.
yes, but i modify because it is only one path.
india_banking/hooks.py
Outdated
| } | ||
|
|
||
| doc_events = { | ||
| "Bank": {"on_trash": "india_banking.india_banking.doc_events.bank.bank_on_trash"}, |
There was a problem hiding this comment.
Can we rename this function as disallow_standard_bank_deletion
| @@ -8,7 +8,8 @@ frappe.ui.form.on("Bank Connector", { | |||
| filters: { | |||
| disabled: 0, | |||
| is_default: 1, | |||
| is_company_account: 1 | |||
| is_company_account: 1, | |||
| company: doc.company, | |||
There was a problem hiding this comment.
Please use the standard linter and rectify the spacing for the entire repository.
|
|
||
|
|
||
| def validate_ifsc_code(doc, method=None): | ||
| if not IFSC_PATTERN.match(cstr(doc.branch_code)): |
There was a problem hiding this comment.
User often get confused with branch code and IFSC code. Can we set the label as IFSC Code for this field if this app is installed?
chore: rename 'bank_on_trash' to 'disallow_standard_bank_deletion' fix: update maximum limit for all mode of transfer chore: rename 'Branch Code' label to 'IFSC Code' in bank account doc
| ] | ||
|
|
||
| STD_BANK_LIST = [ | ||
| "HDFC Bank", |
There was a problem hiding this comment.
Why is Axis Bank, removed?
india_banking/install.py
Outdated
| "insert_after": "posting_date", | ||
| }, | ||
| { | ||
| "label": "ICICI Bank Api Info", |
There was a problem hiding this comment.
Bank specific fields can be avoided. What shall we do here?
| "fieldname": "grand_total", | ||
| "property": "reqd", | ||
| "property_type": "Check", | ||
| "value": 0, |
There was a problem hiding this comment.
Why is it set to 0?, Is the grand_total not mandatory?
There was a problem hiding this comment.
The grand total will be calculated using the net total. we keep net total as mandatory
india_banking/install.py
Outdated
|
|
||
| def create_default_payment_type(): | ||
| if not frappe.db.exists("Payment Type", "Pay"): | ||
| frappe.get_doc({"doctype": "Payment Type", "payment_type": "Pay"}).insert( |
There was a problem hiding this comment.
Instead of ignoring mandatory, can we not get the default accounts payable account from the company and set it to the payment type?
| function get_bank_query_conditions(frm) { | ||
| let conditions = { | ||
| company: frm.doc.company, |
There was a problem hiding this comment.
Would you also like to consider the workflow state as approved here?
And is company filter required? Does bank account have company and is it mandatory?
| "You cannot cancel a payment order with Initiated/Processed payments" | ||
| ) | ||
| ) | ||
| super().on_cancel() |
There was a problem hiding this comment.
on_cancel is already there in the ERPNext app. They are using on_cancel to update the status.
| from frappe import _ | ||
|
|
||
|
|
||
| class CustomPaymentEntry(PaymentEntry): |
There was a problem hiding this comment.
As you're not overriding anything here, then this class and file is not required? Can you move the below code elsewhere and remove this class and file.
| ) | ||
| .where( | ||
| (JournalEntryAccount.parent == source.name) | ||
| & (JournalEntryAccount.party_type == "Employee") |
There was a problem hiding this comment.
Does it mean, we do not allow any supplier, customer bank entry type journal entries?
| "account", | ||
| "cost_center", | ||
| "project", | ||
| "debit as amount", |
There was a problem hiding this comment.
Assuming debit and debit in account currency as same here? Can you add currency as INR as a validation in the query?
| frappe.db.commit() |
There was a problem hiding this comment.
Why are you manually commiting?
There was a problem hiding this comment.
To avoid request and response data loss while processing the response, some time will raise an error.
Vigneshsekar
changed the title
refactor: customisation added
No description provided.